Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Public Cache API and test #6550

Merged
merged 1 commit into from
Apr 3, 2021
Merged

Public Cache API and test #6550

merged 1 commit into from
Apr 3, 2021

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented Feb 6, 2021

Make Cache API public, and test capturing events.

@yschimke yschimke changed the title Public API and test Public Cache API and test Feb 6, 2021
@yschimke yschimke requested a review from swankjesse February 6, 2021 16:54
@yschimke
Copy link
Collaborator Author

yschimke commented Feb 7, 2021

This API doesn't address the handling of the unexplained runtime exceptions due to file corruption/race conditions/multi process etc.

But it raises a few things to consider

  1. Should we have unit tests for corruption cases.
  2. Should we build to provably recover or consider an API to report them? (corrupted count or onCorruption).
  3. Should we provide a sample to write your own decorated filesystem that avoids the multi instance or process cases? e.g. a filesystem that writes out the process name and pid and clears up under normal uses and logs if there is a mismatch?

3 is a half baked idea, but how do we help clients be aware that something is broken?

@swankjesse
Copy link
Collaborator

I'd love to figure out the definitive cause here.

If it's multiple processes overlapping? I bet there's probably some patterns here we haven't considered to detect it, and then we'd need a strategy to recover if that happens. Perhaps not cache on the 2nd instance? Yuck.

If it's multiple instances in the same process? We should probably detect this through a static variable and throw an exception?

@yschimke
Copy link
Collaborator Author

yschimke commented Feb 7, 2021

What about combining the processes and instances. An Android gist with a FileSystem that tries to enforce uniqueness by soft-locking on "appname:pid:instancecounter", and open and close write and delete the market file. Report by default, but optional fail on startup during dev?

class SelfishFilesystem(appCtxt: ApplicationContext, pid = ..., counter = counter.incrementAndGet()): Filesystem {
...
}

@yschimke
Copy link
Collaborator Author

yschimke commented Mar 5, 2021

@swankjesse are you for exposing this in 5.0?

@swankjesse
Copy link
Collaborator

Yes!

@yschimke
Copy link
Collaborator Author

yschimke commented Mar 6, 2021

OK, holding off for approval. This change is just that minimal public API.

class Cache(
   directory: Path,
   maxSize: Long,
   fileSystem: FileSystem
)

And a useful test with example logging that should be easy to build new integrating cases off.

@yschimke
Copy link
Collaborator Author

yschimke commented Apr 2, 2021

@swankjesse ping


c.size();

assertThat(events).containsExactly("metadataOrNull:/cache/journal.bkp",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it.

okhttp/src/test/java/okhttp3/CacheTest.java Outdated Show resolved Hide resolved
okhttp/src/test/java/okhttp3/CacheTest.java Outdated Show resolved Hide resolved
Public API and test

Public API and test

Review comments

Public API and test

Public API and test

Public API and test

Review comments
@yschimke yschimke merged commit a333292 into square:master Apr 3, 2021
@yschimke yschimke deleted the cacheapi branch April 16, 2021 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants